Conversation
arturobernalg
force-pushed
the
rfc7639
branch
2 times, most recently
from
42fd028 to
6140646
Compare
arturobernalg
force-pushed
the
rfc7639
branch
from
4b95aca to
8f872b6
Compare
arturobernalg
force-pushed
the
rfc7639
branch
from
aea3fa3 to
5fa9a19
Compare
arturobernalg
force-pushed
the
rfc7639
branch
from
5fa9a19 to
16b5442
Compare
|
@arturobernalg Could you please rebase this change-set? |
arturobernalg
force-pushed
the
rfc7639
branch
2 times, most recently
from
cc58cd1 to
208b565
Compare
@ok2c done. |
arturobernalg
force-pushed
the
rfc7639
branch
from
208b565 to
eed86b1
Compare
httpclient5/src/main/java/org/apache/hc/client5/http/impl/AlpnHeaderSupport.java
Show resolved
Hide resolved
| /** | ||
| * Formats a list of raw ALPN protocol IDs into a single {@code ALPN} header value. | ||
| */ | ||
| public static String formatValue(final List<String> protocolIds) { |
There was a problem hiding this comment.
@arturobernalg Use MessageSupport#headerOfTokens instead
| /** | ||
| * Parses an {@code ALPN} header value into decoded protocol IDs. | ||
| */ | ||
| public static List<String> parseValue(final String value) { |
There was a problem hiding this comment.
@arturobernalg Use MessageSupport#parseTokens(Header, Consumer) instead.
arturobernalg
force-pushed
the
rfc7639
branch
from
733a98c to
a910585
Compare
| tokens.add(encodeId(id)); | ||
| } | ||
| final Header header = MessageSupport.headerOfTokens(HEADER_NAME, tokens); | ||
| return header.getValue(); |
There was a problem hiding this comment.
@arturobernalg Noooooooo, please do not do that! You create a header with a char buffer, than you you get the value of it as a string, then you make char buffer with a copy of the string and then create a header. Please do not do that.
Please focus more on quality of your contributions than their volume,
My other two comments have not been addressed.
There was a problem hiding this comment.
@ok2c sorry about that. I think all are fine now
arturobernalg
force-pushed
the
rfc7639
branch
2 times, most recently
from
a178fe7 to
e343841
Compare
| * Parses an {@code ALPN} header into decoded protocol IDs. | ||
| */ | ||
| public static List<String> parseValue(final Header header) { | ||
| if (header == null) { |
There was a problem hiding this comment.
@arturobernalg This is an internal method. It does not need to do strict input validation. This is the responsibility of public methods. Keep it simple and small.
| /** | ||
| * Encodes a single raw protocol ID to canonical token form. | ||
| */ | ||
| public static String encodeId(final String id) { |
There was a problem hiding this comment.
@arturobernalg I asked this already, I have to ask it again. What is the reason for having to re-implement percent coding and not using PercentCodec from core? What is wrong with PercentCodec encoding?
There was a problem hiding this comment.
@ok2c RFC 7639 requires canonical token encoding: octets MUST NOT be percent-encoded if they are valid RFC 7230 tchar (except ‘%’), and percent-encoding MUST use uppercase hex.
There was a problem hiding this comment.
@arturobernalg There are two publicly accessible instances and they both do not fit the requirements?
public static final PercentCodec RFC3986 = new PercentCodec(UNRESERVED);
public static final PercentCodec RFC5987 = new PercentCodec(RFC5987_UNRESERVED);
percent-encoding MUST use uppercase hex
This I do not quite understand. Can you give me an example of what PercentCodec produces and what this new RFC requires?
There was a problem hiding this comment.
@arturobernalg At any rate, could you contribute the correct codec to core? That would be the proper way to start.
| try { | ||
| return PercentCodec.decode(token, StandardCharsets.UTF_8); | ||
| } catch (final IllegalArgumentException ex) { | ||
| return token; |
There was a problem hiding this comment.
@arturobernalg This is a protocol error. Should not we treat it as such?
arturobernalg
force-pushed
the
rfc7639
branch
from
2424727 to
b17b351
Compare
Emit ALPN on CONNECT tunnels from classic and async exec when configured. Use canonical percent-encoding with liberal decoding for protocol IDs.
arturobernalg
force-pushed
the
rfc7639
branch
from
249924c to
bfb72b3
Compare
2 participants
Add ConnectAlpnProvider and inject ALPN header in ConnectExec/AsyncConnectExec. Provide builder hooks for fixed list or provider-driven values.